WIP: redesign arg parsing with structopt #52
Conversation
PaulGrandperrin
force-pushed
the
structopt
branch
from
48318f7
to
b8951b9
Compare
| # requires the `cli` feature, but if it is forgotten, we want to fail compilation instead of silently skipping this binary | ||
| # we use conditionnal compilation to throw the error |
There was a problem hiding this comment.
Not sure I would want to go this way, I'd prefer to split it into two crates and avoid the end user chores.
There was a problem hiding this comment.
IDK, maintaining two crates, their synchronization, and making their doc reference each other in a non confusing way seems difficult too.
It would make sense to name the binary crate hfuzz though..
But the feature flag is not so bad, the user will try cargo install honggfuzz, will get a clear error stating that the flag is missing, and try again with cargo install honggfuzz --features=cli.
In the docs, the later command will be directly shown.
src/bin/cargo-hfuzz.rs
Outdated
| input: String, | ||
|
|
||
| /// which binary target to fuzz | ||
| target: String, |
There was a problem hiding this comment.
I'd be in favour of changing this to --bin, since target is rather ambiguous with cargo having --target <TRIPLE>... Build for the target triple.
There was a problem hiding this comment.
ah yes, target is indeed confusing.
however, as of now, this is not an optional argument (a la --bin), but we could rename it as binary and it would be reflected in the help messages.
WDYT?
src/bin/cargo-hfuzz.rs
Outdated
| #[structopt(long)] | ||
| no_instr: bool, | ||
|
|
||
| /// args to target -- args to fuzzer |
There was a problem hiding this comment.
Provides additional arguments to the underlying honggfuzz binary. See [honggfuzz usage](https//..) for details.
There was a problem hiding this comment.
right now, the rightmost args given on the command line are forwarded to the fuzzed binary (child).
that's why to give args to the fuzzer (parent), it is currently needed to put them in an ENV var.
To keep that functionality, I wanted to introduce using -- as a way to separate the two kinds of args.
But maybe, the args to the fuzzed binary are not often used, if ever. Maybe it could be sacrificed in order to make things simpler.
src/bin/cargo-hfuzz.rs
Outdated
|
|
||
| /// args to target -- args to fuzzer | ||
| /// ( https://github.com/google/honggfuzz/blob/master/docs/USAGE.md ) | ||
| args: Vec<String>, |
There was a problem hiding this comment.
I'd like to have a few additional options here, that provide convenience i.e. --iterations=<N> and --exit-on-crash=<code> - these are very common for CI usage, so shortcuts are a nicety :)
There was a problem hiding this comment.
I don't understand your point, with this new code, those args could be put directly on the command line as you want.
Their documentation is from the upstream honggfuzz binary though...
I'll try to make it easy to access this help from the wrapper.
Maybe something like cargo hfuzz help inner-fuzzer would display the result of honggfuzz -h
There was a problem hiding this comment.
I very much like the outcome of the structopt outline, I did the same but the structure I ended up with was slightly more complex since I tried to retain most of the initial semantics (i.e. keep the same set of commands).
Regarding adding the feature to avoid splitting the crate into two. I can't quite follow why this is prefered over the crate split, could you please shed some light? Thanks!
I tried too at first but I wasn't satisfied with the result.. |
I already partially answered in a previous comment, but I'm open to the idea and I agree that it would make sense. I just want things to be as easy as possible to the user. |
PaulGrandperrin
force-pushed
the
master
branch
3 times, most recently
from
badb752
to
cfdbed3
Compare
@drahnr, I played a little bit with how we could redesign the parsing of arguments.
I shuffled most of the code in a module in order to be able to not pull
structoptas a dependency to the usage of the crate as a library.Another solution could be to split the crate in two:
honggfuzz-libandhonggfuzz-binbut I'd rather not.